-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add HttpProtocol enum and http_protocol field to Message #21
Conversation
This commit adds a new `HttpProtocol` enum that represents HTTP versions and adds an optional `http_protocol` field to the `Message` message. This allows specifying the particular HTTP version used if the DOH protocol is in use. Hypothetically, this field could be used in the future to specify the HTTP version in use for an HTTP-based DNS transport protocol that is not the currently existing RFC 8484 DOH protocol. However, at the time of this writing none of the other currently defined dnstap `SocketProtocol` values are HTTP-based transports, so the `http_protocol` field should only be set if `socket_protocol` is set to DOH. Otherwise, the `http_protocol` field should be left unset.
This addresses #20. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me (DNSdist developer here), thanks!
I would suggest adding this as a new message for protocol metadata so that it can be extended to other protocols, existing and future ones. Example: message ProtocolMetadata {
optional HttpProtocol http_protocol ...
optional TLSVersion tls_version ...
optional QUICVersion quic_version ...
...
}
message Message {
...
optional ProtocolMetadata protocol_metadata = 16;
...
} |
Why? This is no more extensible than adding those fields directly to |
It's easier to find, read, understand, document and consumers that doesn't care for protocol metadata could skip it altogether. But sure, they can be put in |
All of the fields in the
Sure, but they still pay the cost of the extra framing (handful of extra bytes per payload, plus allocs, when present).
I agree, it would also be nice to capture TLS and QUIC versions if those protocols are in use. |
This commit adds a new
HttpProtocol
enum that represents HTTP versions and adds an optionalhttp_protocol
field to theMessage
message. This allows specifying the particular HTTP version used if the DOH protocol is in use.Hypothetically, this field could be used in the future to specify the HTTP version in use for an HTTP-based DNS transport protocol that is not the currently existing RFC 8484 DOH protocol. However, at the time of this writing none of the other currently defined dnstap
SocketProtocol
values are HTTP-based transports, so thehttp_protocol
field should only be set ifsocket_protocol
is set to DOH. Otherwise, thehttp_protocol
field should be left unset.